refact(core): optimized batch removal of remaining indices consumed by a single consumer #2203

zyxxoo · 2023-04-23T07:58:00Z

No description provided.

codecov · 2023-04-23T09:52:38Z

Codecov Report

Merging #2203 (d087eca) into master (f23c648) will increase coverage by 4.18%.
The diff coverage is 64.34%.

@@             Coverage Diff              @@
##             master    #2203      +/-   ##
============================================
+ Coverage     64.39%   68.58%   +4.18%     
- Complexity      678      979     +301     
============================================
  Files           497      498       +1     
  Lines         40573    40681     +108     
  Branches       5663     5681      +18     
============================================
+ Hits          26129    27902    +1773     
+ Misses        11828    10074    -1754     
- Partials       2616     2705      +89

Impacted Files	Coverage Δ
...raph/backend/store/rocksdb/RocksDBStdSessions.java	`75.64% <0.00%> (+6.34%)`	⬆️
...ugegraph/backend/store/raft/StoreSnapshotFile.java	`41.88% <50.00%> (+41.88%)`	⬆️
.../hugegraph/backend/store/rocksdb/RocksDBStore.java	`73.46% <50.00%> (+30.50%)`	⬆️
...a/org/apache/hugegraph/task/EphemeralJobQueue.java	`62.50% <62.50%> (ø)`
...he/hugegraph/backend/tx/GraphIndexTransaction.java	`83.36% <71.42%> (-0.36%)`	⬇️
...n/java/org/apache/hugegraph/StandardHugeGraph.java	`69.15% <100.00%> (+1.48%)`	⬆️
...hugegraph/backend/store/raft/rpc/RpcForwarder.java	`70.96% <100.00%> (+70.96%)`	⬆️
...n/java/org/apache/hugegraph/util/CompressUtil.java	`87.25% <100.00%> (ø)`
...hugegraph/backend/store/rocksdb/OpenedRocksDB.java	`73.33% <100.00%> (ø)`
...h/backend/store/rocksdbsst/RocksDBSstSessions.java	`47.65% <100.00%> (ø)`

... and 76 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

zyxxoo · 2023-04-23T10:06:51Z

optimize #2081 #2201

hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphIndexTransaction.java

javeme · 2023-04-23T13:55:28Z

hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphIndexTransaction.java

+            if (this.state.compareAndSet(State.INIT, State.EXECUTE)) {
+                try {
+                    RemoveLeftIndexJob job = new RemoveLeftIndexJob(pendingQueue, this::consumeComplete,
+                                                                    this::reSchedule);


can we just call reSchedule() in consumeComplete()

no, the reSchedule method is to start the job execution when it detects that there is data. The consumeComplete function means that the current job has been completed and the mark needs to be updated. The next time new data enters the queue, the job can be triggered

hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphIndexTransaction.java

hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/EphemeralJobQueue.java

hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphIndexTransaction.java

hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/EphemeralJobQueue.java

hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphIndexTransaction.java

hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/EphemeralJobQueue.java

imbajin

review it soon:)

javeme · 2023-05-06T15:40:41Z

raft api test error:

[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.apache.hugegraph.api.ApiTestSuite
Error:  Tests run: 90, Failures: 1, Errors: 0, Skipped: 1, Time elapsed: 714.611 s <<< FAILURE! - in org.apache.hugegraph.api.ApiTestSuite
Error:  testTruncate(org.apache.hugegraph.api.GremlinApiTest)  Time elapsed: 1.257 s  <<< FAILURE!
java.lang.AssertionError: Response with status 500 and content {"exception":"java.lang.IllegalStateException","message":"The snapshot future can't be null","cause":"[java.lang.IllegalStateException]"} expected:<200> but was:<500>
	at org.apache.hugegraph.api.GremlinApiTest.testTruncate(GremlinApiTest.java:139)

[INFO] 
[INFO] Results:
[INFO] 
Error:  Failures: 
Error:    GremlinApiTest.testTruncate:139->BaseApiTest.assertResponseStatus:615 Response with status 500 and content {"exception":"java.lang.IllegalStateException","message":"The snapshot future can't be null","cause":"[java.lang.IllegalStateException]"} expected:<200> but was:<500>
[INFO] 
Error:  Tests run: 90, Failures: 1, Errors: 0, Skipped: 1

zyxxoo · 2023-05-08T02:32:42Z

GremlinApiTest

public void forwardToLeader(PeerId leaderId, StoreCommand command,
                              RaftStoreClosure future) {
      E.checkNotNull(leaderId, "leader id");
      E.checkState(!leaderId.equals(this.nodeId),
                   "Invalid state: current node is the leader, there is " +
                   "no need to forward the request");
      LOG.debug("The node {} forward request to leader {}",
                this.nodeId, leaderId);

      StoreCommandRequest.Builder builder = StoreCommandRequest.newBuilder();
      builder.setType(command.type());
      builder.setAction(command.action());
      builder.setData(ZeroByteStringHelper.wrap(command.data()));
      StoreCommandRequest request = builder.build();

      RpcResponseClosure<StoreCommandResponse> responseClosure;
      responseClosure = new RpcResponseClosure<StoreCommandResponse>() {
          @Override
          public void setResponse(StoreCommandResponse response) {
              if (response.getStatus()) {
                  LOG.debug("StoreCommandResponse status ok");
                  future.complete(Status.OK(), () -> null);
              } else {
                  LOG.debug("StoreCommandResponse status error");
                  Status status = new Status(RaftError.UNKNOWN,
                                             "fowared request failed");
                  BackendException e = new BackendException(
                                       "Current node isn't leader, leader " +
                                       "is [%s], failed to forward request " +
                                       "to leader: %s",
                                       leaderId, response.getMessage());
                  future.failure(status, e);
              }
          }

          @Override
          public void run(Status status) {
              future.run(status);
          }
      };
      this.waitRpc(leaderId.getEndpoint(), request, responseClosure);
  }

the code future.complete(Status.OK(), () -> null); return a null future, Maybe be the place should put a future that complete with null value

zyxxoo · 2023-05-08T06:37:30Z

java 11 rocksdb block
when i run ApiTestSuit on local env, the test will block on CypherApiTest#testRelationQuery;
but if i only run CypherApiTest on ApiTestSuit(Comment other classes), the running will not block or block on CypherApiTest other test method

hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/EphemeralJobQueue.java

hugegraph-core/src/main/java/org/apache/hugegraph/task/TaskCallable.java

javeme · 2023-05-08T10:25:52Z

hugegraph-core/src/main/java/org/apache/hugegraph/util/CompressUtil.java

@@ -145,7 +145,7 @@ public static void decompressTar(String sourceFile, String outputDir,
                    Files.createDirectories(newPath);
                } else {
                    // check parent folder again
-                    Path parent = newPath.getParent();
+                    Path parent = newPath.toAbsolutePath().getParent();


can we split into a separated commit?

I mainly did it for convenience, putting two git commits together is simpler.

hugegraph-core/src/main/java/org/apache/hugegraph/task/TaskCallable.java

hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/EphemeralJobQueue.java

javeme · 2023-05-08T11:12:16Z

hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/EphemeralJobQueue.java

+            return ret;
+        }
+
+        private Object executeBatchJob(List<EphemeralJob<?>> jobs, Object prev) throws Exception {


can keep List<Job<?>>?

The main purpose of the queue is to batch EphemeralJob. The name EphemeralJob is preferred and the job's parameters must be set during initialization (the SysTaskCallable defines the params method).

hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/EphemeralJobQueue.java

hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/raft/rpc/RpcForwarder.java

hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/EphemeralJobQueue.java

javeme · 2023-05-09T11:05:17Z

hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/EphemeralJobQueue.java

+                if (e instanceof InterruptedException) {
+                    Thread.currentThread().interrupt();
+                    if (queue != null) {
+                        queue.queue().clear();


not sure why called clear() here.
and prefer to call queue.clear().

When an InterruptedException occurs, these tasks may be aborted, so clear all tasks

hugegraph-core/src/main/java/org/apache/hugegraph/security/HugeSecurityManager.java

hugegraph-core/src/main/java/org/apache/hugegraph/task/TaskCallable.java

hugegraph-core/src/main/java/org/apache/hugegraph/task/EphemeralJobQueue.java

javeme · 2023-05-11T07:09:05Z

hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphIndexTransaction.java

-                                              .job(job)
-                                              .schedule();
-        return task.id();
+        job.selfCommit(false);


does only RemoveLeftIndexJob class contain the selfCommit() method? if not then we can move selfCommit() call into submitEphemeralJob().

I think we should handle everything in batches to keep it simple. Let's exclude 'selfCommit' for now.

javeme · 2023-05-11T07:14:19Z

hugegraph-core/src/main/java/org/apache/hugegraph/task/EphemeralJobQueue.java

+
+        private static final long PAGE_SIZE = Query.COMMIT_BATCH;
+        private static final String BATCH_EPHEMERAL_JOB = "batch-ephemeral-job";
+        private static final int MAX_CONSUME_COUNT = EphemeralJobQueue.CAPACITY / 2;


now COMMIT_BATCH is 100, CAPACITY is 1000.
should we let CAPACITY >> COMMIT_BATCH and MAX_CONSUME_COUNT ~= PAGE_SIZE?
such as let MAX_CONSUME_COUNT = COMMIT_BATCH, let CAPACITY = 100 * COMMIT_BATCH

do you mean remove "PAGE_SIZE" field?

I change to below formula:
CAPACITY = 100 * Query.COMMIT_BATCH;
PAGE_SIZE = Query.COMMIT_BATCH;
MAX_CONSUME_COUNT = 2 * PAGE_SIZE;

hugegraph-core/src/main/java/org/apache/hugegraph/task/EphemeralJobQueue.java

hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphTransaction.java

hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/IndexableTransaction.java

hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/SchemaTransaction.java

hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphIndexTransaction.java

javeme · 2023-05-23T11:23:56Z

hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphIndexTransaction.java

@@ -1780,7 +1776,6 @@ protected long removeIndexLeft(ConditionQuery query,
                // Process secondary index or search index
                sCount += this.processSecondaryOrSearchIndexLeft(cq, element);
            }
-            this.tx.commit();


so we don't support auto commit anymore?

imbajin · 2023-05-31T04:35:48Z

hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/raft/rpc/RpcForwarder.java

+                    // This code forwards the request to the Raft leader and considers the operation successful
+                    // if it's forwarded successfully. It returns a RaftClosure because the calling
+                    // logic expects a RaftClosure result. Specifically, if the current instance is the Raft leader,
+                    // it executes the corresponding logic locally and notifies the calling logic asynchronously
+                    // via RaftClosure. Therefore, the result is returned as a RaftClosure here.


maybe better to use /* */ next time

zyxxoo force-pushed the zy_dev branch from 9fc12c1 to 9adf0d3 Compare April 23, 2023 09:40

zyxxoo force-pushed the zy_dev branch 3 times, most recently from 53d99a1 to 8441b2a Compare April 23, 2023 09:58

zyxxoo changed the title ~~single task consume batch remove left index task~~ chore: single task consume batch remove left index task Apr 23, 2023

zyxxoo requested review from imbajin and javeme April 23, 2023 09:59

zyxxoo changed the title ~~chore: single task consume batch remove left index task~~ optimized: batch removal of remaining indices consumed by a single consumer" Apr 23, 2023

github-advanced-security bot found potential problems Apr 23, 2023

View reviewed changes

hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphIndexTransaction.java Fixed Show fixed Hide fixed

hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphIndexTransaction.java Fixed Show fixed Hide fixed

javeme reviewed Apr 23, 2023

View reviewed changes

imbajin changed the title ~~optimized: batch removal of remaining indices consumed by a single consumer"~~ refact(core): optimized batch removal of remaining indices consumed by a single consumer Apr 23, 2023

javeme reviewed May 4, 2023

View reviewed changes

javeme reviewed May 5, 2023

View reviewed changes

zyxxoo force-pushed the zy_dev branch from 10d7a51 to 788a948 Compare May 5, 2023 08:27

javeme previously approved these changes May 5, 2023

View reviewed changes

imbajin reviewed May 5, 2023

View reviewed changes

zyxxoo dismissed javeme’s stale review via d5830a7 May 8, 2023 03:53

zyxxoo force-pushed the zy_dev branch from d5830a7 to 18f7423 Compare May 8, 2023 04:03

zyxxoo force-pushed the zy_dev branch 2 times, most recently from 2e76d4b to 16f6934 Compare May 8, 2023 07:29

javeme reviewed May 8, 2023

View reviewed changes

javeme reviewed May 9, 2023

View reviewed changes

zyxxoo force-pushed the zy_dev branch from aa87b91 to 79c00ea Compare May 10, 2023 01:56

javeme reviewed May 10, 2023

View reviewed changes

hugegraph-core/src/main/java/org/apache/hugegraph/task/EphemeralJobQueue.java Show resolved Hide resolved

javeme reviewed May 11, 2023

View reviewed changes

javeme previously approved these changes May 16, 2023

View reviewed changes

zyxxoo dismissed javeme’s stale review via 179a1da May 19, 2023 09:07

zyxxoo force-pushed the zy_dev branch from 179a1da to 3539c53 Compare May 19, 2023 09:15

javeme reviewed May 19, 2023

View reviewed changes

zyxxoo added 12 commits May 22, 2023 10:33

chore: single task and batch consume remove left index task

d837517

improve code

e29bcdd

improve

ec857a3

improve

134cd52

improve

616c64d

improve

d834204

improve

83923bb

fix: rocksdb

a124f30

improve

64cf79e

improve

8ed14d8

improve

df28ad9

improve

d087eca

zyxxoo force-pushed the zy_dev branch from 3539c53 to d087eca Compare May 22, 2023 02:39

javeme approved these changes May 23, 2023

View reviewed changes

imbajin approved these changes May 31, 2023

View reviewed changes

imbajin merged commit e1d9607 into master May 31, 2023

imbajin deleted the zy_dev branch May 31, 2023 04:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refact(core): optimized batch removal of remaining indices consumed by a single consumer #2203

refact(core): optimized batch removal of remaining indices consumed by a single consumer #2203

zyxxoo commented Apr 23, 2023

codecov bot commented Apr 23, 2023 •

edited

Loading

zyxxoo commented Apr 23, 2023 •

edited

Loading

javeme Apr 23, 2023

zyxxoo Apr 24, 2023

imbajin left a comment •

edited

Loading

javeme commented May 6, 2023

zyxxoo commented May 8, 2023 •

edited

Loading

zyxxoo commented May 8, 2023 •

edited

Loading

javeme May 8, 2023

zyxxoo May 9, 2023 •

edited

Loading

javeme May 8, 2023

zyxxoo May 9, 2023 •

edited

Loading

javeme May 9, 2023

zyxxoo May 10, 2023 •

edited

Loading

javeme May 11, 2023

zyxxoo May 19, 2023

javeme May 11, 2023

zyxxoo May 19, 2023

zyxxoo May 19, 2023

javeme May 23, 2023

imbajin May 31, 2023

refact(core): optimized batch removal of remaining indices consumed by a single consumer #2203

refact(core): optimized batch removal of remaining indices consumed by a single consumer #2203

Conversation

zyxxoo commented Apr 23, 2023

codecov bot commented Apr 23, 2023 • edited Loading

Codecov Report

zyxxoo commented Apr 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imbajin left a comment • edited Loading

Choose a reason for hiding this comment

javeme commented May 6, 2023

zyxxoo commented May 8, 2023 • edited Loading

zyxxoo commented May 8, 2023 • edited Loading

Choose a reason for hiding this comment

zyxxoo May 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyxxoo May 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyxxoo May 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 23, 2023 •

edited

Loading

zyxxoo commented Apr 23, 2023 •

edited

Loading

imbajin left a comment •

edited

Loading

zyxxoo commented May 8, 2023 •

edited

Loading

zyxxoo commented May 8, 2023 •

edited

Loading

zyxxoo May 9, 2023 •

edited

Loading

zyxxoo May 9, 2023 •

edited

Loading

zyxxoo May 10, 2023 •

edited

Loading